Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ts migration(src/pages): Migrated assigned files #6829

Merged
merged 21 commits into from
Jul 16, 2022

Conversation

M-Ivan
Copy link
Contributor

@M-Ivan M-Ivan commented Jun 26, 2022

Description

Migrated:

  • src/pages/404.js
  • src/pages/assets.js
  • src/pages/bug-bounty.js
  • src/pages/community.js

Related Issue

PR Is related with 6392

@gatsby-cloud
Copy link

gatsby-cloud bot commented Jun 26, 2022

Gatsby Cloud Build Report

ethereum-org-website-dev

🎉 Your build was successful! See the Deploy preview here.

Build Details

View the build logs here.

🕐 Build time: 27m

Copy link
Member

@pettinarip pettinarip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@M-Ivan thanks for the contribution!

Similar feedback as in your other PR. Lets use PageProps + the auto-generated query type.

<StyledPage>
<Content>
<h1>
<Translation id="we-couldnt-find-that-page" />
</h1>
<p>
<Translation id="try-using-search" />{" "}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need that extra space

@@ -10,14 +10,14 @@ const StyledPage = styled(Page)`
margin-top: 4rem;
`

const NotFoundPage = () => (
const NotFoundPage: React.FC = () => (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a Page component, we are not defining it as a React.FC. Lets keep the consistency across the other pages by only defining the props with PageProps.

Suggested change
const NotFoundPage: React.FC = () => (
const NotFoundPage = (props: PageProps) => {

data: Record<string, IGatsbyChildImageSharp>
}

const AssetsPage: React.FC<IProps> = ({ data }) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in the other PR, lets use PageProps from gatsby and use the auto-generated types using Queries.{NameOfTheQuery}.

@@ -116,7 +143,7 @@ const AssetsPage = ({ data }) => {
<AssetDownload
title={translateMessageId("page-assets-hero", intl)}
alt={translateMessageId("page-assets-hero", intl)}
image={data.hero}
image={getImage(data.hero)}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't need to do getImage here since AssetDownload is already doing that for us.

link: string
}

const BugBountiesPage: React.FC<IProps> = ({ data, location }) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as the other pages, lest use PageProps.

@@ -326,7 +361,7 @@ const BugBountiesPage = ({ data, location }) => {
? getImage(data.lighthouseDark)
: getImage(data.lighthouseLight)

const specs = [
const specs: ISpec[] = [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency,

Suggested change
const specs: ISpec[] = [
const specs: Array<ISpec> = [

description: string
}

const CommunityPage: React.FC<IProps> = ({ data }) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, PageProps

@@ -316,7 +335,7 @@ const CommunityPage = ({ data }) => {
alt: translateMessageId("page-community-hero-alt", intl),
}

const cards = [
const cards: ICard[] = [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const cards: ICard[] = [
const cards: Array<ICard> = [

@github-actions github-actions bot added the translation 🌍 This is related to our Translation Program label Jul 9, 2022
@@ -26,7 +26,7 @@ interface IPropsWithSVG extends IPropsBase {
}
interface IPropsWithImage extends IPropsBase {
svg?: never
image: string
image: Record<string, unknown>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pettinarip since this prop is supposed to receive the GatsbyImageData object i changed this type to match any object. It was set to string before and it was a pain to make it work when passing a Gatsby auto-generated type (from the Queries namespace).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is nice! as commented in the other comment, we will leave it as any for now. Once we have better types from gatsby we will refactor and fix all the images types 👍🏼

@@ -117,7 +118,7 @@ const AssetsPage = ({ data }) => {
<AssetDownload
title={translateMessageId("page-assets-hero", intl)}
alt={translateMessageId("page-assets-hero", intl)}
image={data.hero}
image={data.hero as Record<string, unknown>}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pettinarip Type assetion here is for the AssetDownload component prop to don't cry. It actually complains a lot because the props of the component expect a non-nullable object, even tho in the component this specific prop value is used for a gatsby call is using getSrc, a null-safe method (docs here)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine but there are some issues with the getImage types. In the overrides.d.ts we are overriding the types for this function to basically accepts any as its argument. So, in order to keep it simple, we can just accept any in AssetDownload while we wait for a proper fix.

Copy link
Member

@pettinarip pettinarip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@M-Ivan thanks for the PR! this looks great! I'm pushing some minor changes and reverting the md changes.

}

interface ISpec {
title: JSX.Element
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
title: JSX.Element
title: ReactNode

@@ -117,7 +118,7 @@ const AssetsPage = ({ data }) => {
<AssetDownload
title={translateMessageId("page-assets-hero", intl)}
alt={translateMessageId("page-assets-hero", intl)}
image={data.hero}
image={data.hero as Record<string, unknown>}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine but there are some issues with the getImage types. In the overrides.d.ts we are overriding the types for this function to basically accepts any as its argument. So, in order to keep it simple, we can just accept any in AssetDownload while we wait for a proper fix.

@pettinarip pettinarip merged commit 69be560 into ethereum:dev Jul 16, 2022
@corwintines corwintines mentioned this pull request Jul 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
content 🖋️ This involves copy additions or edits translation 🌍 This is related to our Translation Program
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants